Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[WIP] Feature/c++17 #927

Closed
wants to merge 52 commits into from
Closed

Conversation

eugene-so
Copy link

Problem

Upgrade code base to support C++17 compiler.
Using the updated compiler should hopefully allow us to:

  • bring in newer module libraries without constantly facing deprecation issues.
  • use advanced C++17 features
  • development faster and more effectively
  • increase compatibility with the bitcoin core

The bitcoin team began the same transition to C++17 in early 2020 as per bitcoin/bitcoin#16684. The majority of changes brought in will be from their repo.

Root Cause

Compilation issues with OSX Catalina impeding progress
Compilation issues with QT5.15 impeding progress
Unable to bring in new versions of Berkely DB (ie. 5.1.29).

Solution

Surgically bring in well-tested migration commits from the bitcoin repo and refactor veil specific code where necessary.

Bounty PR

Bounty Payment Address

sv1qqp6aptgvgp9t9h8sgzkqmu6cgq2e20l9x6fsl5ask7t3ygy2jagftcpq0x5z0h522ca5h06qq3hx33pke00r7gjt3j24n896gf55y68ptrmjqqqqd8lz3

Unit Testing Results

Will try to bring in changes to unit tests where they were updated for bitcoin.

supere and others added 11 commits April 10, 2021 10:16
Changes in XCode 11 Objective-C Runtime cause an error during build on
MacOS 10.15 Catalina.  Ported solution from bitcoin PR 16720.
Updated instruction on how to build berkeley DB for macOS 10.15
Catalina.
(Ported from bitcoin)

After this:

- `boost::optional` is no longer used directly (only through `Optional`
    which is an alias for it)
- `boost/optional.hpp` is only included in one place
@eugene-so
Copy link
Author

This pull request is in progress. There will likely be a slew of changes, so I created it now to allow any discussion to be added as necessary.

@Rock-N-Troll
Copy link

You can leave the pull request in draft state and also add [WIP] to the pull request title for better clarity of work to be done.

gwillen and others added 16 commits April 11, 2021 22:14
On OS X, when searching Homebrew keg-only packages for BDB 4.8, if we find it,
use BDB_CPPFLAGS and BDB_LIBS instead of CFLAGS and LIBS for the result. This
is (1) more correct, and (2) necessary in order to give this location
priority over other directories in the include search path, which may include
system include directories with other versions of BDB.
This enables of the use of AI_* definitions in the Windows headers,
specifically AI_ADDRCONFIG, which fixes an issue with libevent and
ipv6 on Windows.

It also aligns with what we define in configure when building Core.
libevent uses getaddrinfo when available, and falls back to gethostbyname
Windows has both, but gethostbyname only supports IPv4
libevent fails to detect Windows's getaddrinfo due to not including the right headers
This patches libevent's configure script to check it correctly
@eugene-so eugene-so marked this pull request as draft April 12, 2021 06:51
supere and others added 8 commits April 12, 2021 01:36
HexStr can be called with anything that bas `begin()` and `end()` functions,
so clean up the redundant calls.
Also moves $PTHREAD_CFLAGS to the CFLAGS.
Note that with this change we are no-longer including PTHREAD_* flags
when building libbitcoinconsensus.

Also note that we are including PTHREAD_LIBS in AM_PTHREAD_FLAGS
HexStr can be called with anything that bas `begin()` and `end()` functions,
so clean up the redundant calls.
@Rock-N-Troll
Copy link

I do not envy the thought of doing the work you're doing here. Good work so far though :)

Update macOS installation guide
Reference https://github.com/bitcoin/bitcoin
96124a204193ed114ca9594df7d5151206990e91 thru 48134a09adef3b5302cdd6e95500db404c9ac961
@eugene-so
Copy link
Author

I do not envy the thought of doing the work you're doing here. Good work so far though :)

LOL. Starting to go bug eyed already!!!

@CaveSpectre11
Copy link
Collaborator

I've created a c++17 branch so that we can handle this with multiple PRs to that branch, rather than just having a big giant PR.

@eugene-so
Copy link
Author

I've created a c++17 branch so that we can handle this with multiple PRs to that branch, rather than just having a big giant PR.

Ok. That's great!

fanquake and others added 15 commits April 12, 2021 20:29
This has been around since the introduction of autotools. However at
this point I'm not sure we'd every want to suppress all warnings when
performing a build, and given that CXX FLAGS will have been overriden
when cross-compiling for Windows (using depends), this would rarely,
if-ever be used anyways.

From https://gcc.gnu.org/onlinedocs/gcc/Warning-Options.html:
-w

    Inhibit all warning messages.
Instruct the linker to set the major & minor subsystem versions in the PE
header to 6 & 1 (NT 6.1 which corresponds to Windows 7). Similar to
macOS, the binary will now refuse to run on unsupported versions of
Windows.
This change adds the correct suffix to debug mode .pc filenames for
MinGW and also to the Qt libraries listed in the `Requires` field.
The filename adjustment fixes the accidental overwriting of release
mode .pc files with the debug mode variant which required the wrong
variant of the libraries when `debug_and_release` is active.
Note that macOS also supports the `debug_and_release' configuration
but may use the regular library names together with DYLD_IMAGE_SUFFIX.
Creation of *_debug.pc files is turned off as they're identical to their
non-debug counterparts.
More info:
- QTBUG-4155
- Qt commit a0d8fb4ac3cb7bafdb39f340055eacee4f957513
This change adds to the BITCOIN_QT_CONFIGURE script ability to use
pkg-config for MinGW. All of the non-pkg-config paths are removed as
needless.
If depends is built with DEBUG=1 the configure script fails to pickup
Qt:
- for macOS host (similar, but not the same as issue 16391)
- for Windows host (regression)
QT_STATICPLUGIN is defined in BITCOIN_QT_CONFIGURE macro.
The Mingw-w64 5.0 (Ubuntu 18.04 Bionic) is used to build the Windows
binaries now.
It is safe to use pthread_setname_np since #17538 when the minimal glibc
version is set to 2.17.
@eugene-so eugene-so closed this Apr 13, 2021
@eugene-so
Copy link
Author

Moved to C++17 branch for now

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants